gh-134584: Eliminate redundant refcounting from _CALL_ISINSTANCE#136077
gh-134584: Eliminate redundant refcounting from _CALL_ISINSTANCE#136077noamcohen97 wants to merge 6 commits intopython:mainfrom
_CALL_ISINSTANCE#136077Conversation
noamcohen97
requested review from
Fidget-Spinner,
markshannon and
tomasr8
as code owners
noamcohen97
force-pushed
the
refcount-isinstance
branch
from
55119d3 to
6c04f67
Compare
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Sheesh. This is really good. You even handled the optimizer's constant propagation. Very cool!
We're blocked on Mark's TOS caching PR (see the main issue's comments), so we need to hold off on this for now.
| self.assertNotIn("_POP_CALL_ONE_LOAD_CONST_INLINE_BORROW", uops) | ||
| self.assertNotIn("_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW", uops) | ||
|
|
||
| def test_call_isinstance_guards_pop_top(self): |
There was a problem hiding this comment.
Can't leave a comment on that line, but inside test_call_isinstance_guards_removed we should include the new opcode _SWAP_CALL_TWO_LOAD_CONST_INLINE_BORROW to ensure remove_unneeded_uops is still working as expected (I think it should be since you also updated op_without_push, but just to be sure :))
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
This is very close to getting merged, and in this case I checked the code and there is indeed no refleak (sorry for not checking the other one close enough).
Could you please address the reviews? I can merge it after.
| op(_SWAP_CALL_TWO_LOAD_CONST_INLINE_BORROW, (ptr/4, callable, null, instance, cls -- value, c1, i, c2)) { | ||
| value = PyJitRef_Borrow(sym_new_const(ctx, ptr)); | ||
| c1 = callable; | ||
| i = instance; | ||
| c2 = cls; | ||
| } |
There was a problem hiding this comment.
We don't need this, as the instruction is emitted by the optimizer, and never seen by itself.
| INPUTS_DEAD(); | ||
| c1 = callable; | ||
| i = instance; | ||
| c2 = cls; |
There was a problem hiding this comment.
| INPUTS_DEAD(); | |
| c1 = callable; | |
| i = instance; | |
| c2 = cls; | |
| c1 = callable; | |
| i = instance; | |
| c2 = cls; | |
| INPUTS_DEAD(); |
| } | ||
|
|
||
| op(_CALL_ISINSTANCE, (unused, unused, instance, cls -- res)) { | ||
| op(_CALL_ISINSTANCE, (unused, unused, instance, cls -- res, unused, unused, unused)) { |
There was a problem hiding this comment.
Please name the variables, then assign to them below, e.g.
c1 = callable;
i = instance;
c2 = cls;
|
isinstance is kinda tough to deal with. So I'm closing this for now. |
3 participants
Uh oh!
There was an error while loading. Please reload this page.